refactor(workflows): compaction-resilient recovery and risk-proportional validation (#4560) [DRAFT] - #4562
refactor(workflows): compaction-resilient recovery and risk-proportional validation (#4560) [DRAFT]#4562Yeachan-Heo wants to merge 10 commits into
Conversation
d85f2b5 to
5ebd709
Compare
|
Exact-head CI triage (fix-forward, signed):
— |
|
Terminal exact-head evidence for this Draft (signed): Head
The earlier event-head failures ( This Draft remains owner-controlled: not ready-for-review, not merged, no release/tag/publish. Next step is independent exact-head review and an owner readiness decision. — |
|
Draft hold maintained — owner-controlled reconcile receipt (signed): Submitted state (recorded before any action, unchanged): head Owner-readiness check (pre-mutation): no owner-ready evidence exists. Latest signed evidence on this PR (2026-08-14T17:26:46Z) states the Draft "remains owner-controlled: not ready-for-review, not merged" with next step "an owner readiness decision"; the Delta reconciliation against current dev
Bounded validation on the merged tree: 19 pass (3 new PR suites: workflow-recovery-projection, ultragoal-validation-lanes, agent-session-workflow-recovery-continuation), 258 pass/7 skip (ultragoal-runtime + compaction suites + sdk-kind-aware-reconciliation + notifications-inbound-acceptance), 55 pass (dev-side admission suites: contended-terminal-capture, midrun-maintenance, pre-admission-artifact-spill, sdk-steer-reconciliation, notifications-turn-ordering) — 0 fail; coding-agent typecheck clean ( Hold decision: DRAFT HOLD continues. This PR stays draft and owner-controlled. A rebase onto current dev is validated as clean but intentionally not pushed — exact-head discipline means a push would stale all existing CI/verdict evidence at — |
|
Stale-evidence notice + recomputed draft hold (signed): Base movement: current #4540 overlap inspection: the Recomputed onto exact Bounded validation on the recomputed tree (all fresh): 19 pass (3 new PR suites: workflow-recovery-projection, ultragoal-validation-lanes, agent-session-workflow-recovery-continuation), 258 pass/7 skip (ultragoal-runtime + compaction + sdk-kind-aware-reconciliation + notifications-inbound-acceptance), 57 pass/1 skip (#4540-side: agent-session-auto-compaction-continue, before-agent-start-attribution, manual-retry, sdk/host/session-runtime), 55 pass (admission/reconciliation suites: contended-terminal-capture, midrun-maintenance, pre-admission-artifact-spill, sdk-steer-reconciliation, notifications-turn-ordering) — 0 fail; coding-agent typecheck clean ( Hold decision: DRAFT HOLD continues at unchanged submitted head. No push, no ready-mark, no merge, no release mutation. Blocker remains the owner readiness decision; per exact-head discipline, any future rebase push would stale CI/verdict evidence at — |
|
Owner census — exact live state (2026-08-15):
Disposition in progress: reconstruct the intended change onto current — |
6727450 to
59b3cd6
Compare
|
Owner-state update:
— |
|
@HaD0Yun exact-head review is now the only merge gate. Authoritative head/base: Please submit an — |
|
Exact-head disposition — owner-controlled hold with third-party review gate (signed): Exact live state (recomputed 2026-08-17):
Base movement & fresh trial merge (owner verification, this lane):
Blocker (owner-only, exact): Why no fix-forward push: the exact head is green and merge-clean against live dev; any new push would invalidate the verified exact-head CI/digest/evidence chain without fixing anything. Rebase would be pure churn. Terminal disposition: hold as owner-controlled DRAFT pending the exact-head — |
|
Issue-lane → PR-lane coordination contract (from #4560, signed; issue-side evidence at 5333388850): Current exact live state (this issue-lane census, moments ago): head Remaining path to merge, in order — all PR-lane/owner/reviewer authorities, none issue-lane:
No activity from this issue lane will touch this branch, its verdict, its draft state, or its merge button. This comment records linkage and the exact remaining blocker chain only. — |
b488e23 to
7f42da1
Compare
|
Disposition: RECONSTRUCTED (not superseded). Signed terminal-lane evidence. Prior lane check: head Value check (against
Reconstruction (rebase of all 6 commits onto
Verification on the rebased head:
The PR remains DRAFT; un-drafting / merge review is the maintainer's next call. |
7f42da1 to
4f9f104
Compare
|
Transplant receipt (signed): the preserved implementation was transplanted onto exact current
Next: exact-head CI (this push), then — |
17ce937 to
4247560
Compare
|
Final transplant receipt (signed) — contract-exact state:
Next: exact-head CI on this push; then — |
4247560 to
af4f32a
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@snowykr approval is requested for the exact pushed head |
0fd9da7 to
ceefe4f
Compare
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The direction matches #4560, but the implementation does not yet preserve the advertised intent-gating, risk-proportional validation, and compaction-recovery contracts. In particular, recovery can bypass the new Ralplan intent phase, multi-goal and security-sensitive changes can be downgraded incorrectly, the reduced lane path still requires architect evidence, and durable recovery does not retain in-flight cohort state. There are also filesystem trust/race issues in recovery and source snapshotting.
Findings / Required Changes
-
[P1] Ralplan recovery bypasses mandatory intent reconciliation —
packages/coding-agent/src/gjc-runtime/workflow-recovery-projection.ts:305
A planner-only run is projected asrun-plan-review, even though the new manifest and bundled skill requireplanner -> intentbefore Architect/Critic consensus. A compaction in that window therefore resumes review against an unreconciled draft. Map a latestplannerstage toreconcile-intent; emitrun-plan-reviewonly after a durable intent receipt, and update the forced-compaction regression. -
[P1] Multi-goal risk disappears at the final aggregate checkpoint —
packages/coding-agent/src/gjc-runtime/ultragoal-validation-policy.ts:142
multiGoalis derived from the number of remaining goals. At the last goal of a multi-goal aggregate, that becomes false, allowing reduced validation despite the contract that multi-goal runs remain high risk. Derive this signal from the durable aggregate shape (required/non-superseded goals), and cover the final goal of a two-goal run. -
[P1] The reduced cohort contract still mandates architect approval —
packages/coding-agent/src/gjc-runtime/ultragoal-runtime.ts:2777-2856
The normal workflow has no supported way to obtain the runtime lane-selection proof, and even afterlowRiskReducedis accepted,architectReviewis still unconditionally required and fully validated. This makes the advertised architect omission unusable without running or fabricating architect evidence. Expose the authoritative computed proof through a supported command/response, update the bundled skill to consume it, and make architect evidence conditional on architect applicability. -
[P1] High-risk classification misses real authentication and credential surfaces —
packages/coding-agent/src/gjc-runtime/ultragoal-validation-policy.ts:53
The prefix list omits security-sensitive files such aspackages/ai/src/auth-storage.ts,packages/coding-agent/src/runtime-mcp/oauth-flow.ts, andpackages/coding-agent/src/commands/auth-broker.ts. A single-goal auth change can therefore receive reduced validation. Use a complete shared security/public-contract classifier or expand the fail-closed set with representative regression cases. -
[P1] In-flight boundary-cohort state is not durably recoverable —
packages/coding-agent/src/gjc-runtime/workflow-recovery-projection.ts:435
Recovery only discovers cohort information in completedgoal_checkpointedreceipts. Compaction during parallel cleaner/architect/QA work loses the generation, frozen hash, dispatched/completed lanes, and unjoined findings, which can duplicate work or lose blockers. Persist cohort-start and per-lane completion events, then project an exact await/join/rerun action that dispatches only missing lanes. -
[P1] Recovery path confinement does not reject symlinked ancestors —
packages/coding-agent/src/gjc-runtime/workflow-recovery-projection.ts:182
Only the leaf run directory is checked withlstat; symlinks in session/plans/ralplan ancestors can move the effective root outside the expected tree while still satisfying the relative realpath check. Anchor recovery beneath a canonical symlink-free session plans root and verify every path component with no-follow semantics. -
[P1] Recovery hashes different bytes from those it projects —
packages/coding-agent/src/gjc-runtime/workflow-recovery-projection.ts:263
The artifact is read for projection and then reopened for hashing. A concurrent replacement can make the verified digest correspond to benign bytes while different bytes enter the continuation prompt. Open once with no-follow semantics, hash the same buffer that is decoded/projected, and verify descriptor identity/type. -
[P1] Completion can be committed against a stale source snapshot —
packages/coding-agent/src/gjc-runtime/ultragoal-change-set.ts:273
Name-status, tracked diffs, and untracked files are captured independently, and the resulting snapshot is retained through asynchronous validation without a final recapture. Concurrent repository changes can therefore bypass the frozen-source basis. Build a consistency-checked snapshot, recapture immediately after validation, require the full digest to match, and abort/retry on drift. -
[P1] Required forced-compaction scenarios are not exercised —
packages/coding-agent/test/agent-session-workflow-recovery-continuation.test.ts:176
The session-level suite covers only ordinary Ultragoal implementation and a Ralplan planner artifact. #4560 explicitly requires no/one/repeated compaction across Ralplan review, parallel executor work, parallel boundary review, and blocker-fix/re-review. Add AgentSession-level scenarios that assert generation/source identity, join state, no duplicate lane dispatch, and bounded zero-progress behavior. -
[P2] A failed active-state refresh can reuse a stale workflow owner —
packages/coding-agent/src/session/agent-session.ts:11846
#lastCompactionActiveSkillsis only replaced after a successful read; the catch retains the prior snapshot and the projector consumes it unconditionally. Clear the cache before/on failure or pass the current snapshot directly, and add a success-then-read-failure regression. -
[P2] Validation-lane tests leak
GJC_SESSION_ID—packages/coding-agent/test/gjc-runtime/ultragoal-validation-lanes.test.ts:124
seedPlan()sets the environment variable, but teardown does not restore it. Save and restore/delete the original value to prevent order-dependent shard behavior.
CI / Verification
- Review was performed against exact head
ceefe4fa4f5f96c095838f7d7752c92cd4f6d15a. - Read-only inspection only; PR code was not executed, as required for the verification axis.
- The exact-head
gjc-state-gatesplan, integrity, read, runtime, and static shards are passing. Affected path validation / native-buildwas still pending at submission time.- The PR contract/bootstrap failures explicitly correspond to the expected
needs-humanmerge gate and were not treated as product failures. - The forced-compaction coverage and environment leak above were identified from the changed tests themselves.
Axis Coverage
| Axis | Status | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | Changes requested | Intent phase ordering, risk contract, reduced-lane public contract, scope alignment |
| A2. Architecture / Correctness / Failure | Changes requested | Lifecycle, scheduler/recovery behavior, cohort persistence, source consistency, fallback behavior |
| A3. Security / Privacy / Trust | Changes requested | Persisted-state trust, path confinement, symlink/TOCTOU races, integrity and validation bypasses |
| A4. Verification / Tests / CI | Changes requested | Exact-head CI evidence, changed tests, required compaction matrix, test isolation |
| A5. Context / Compatibility / Platform | Changes requested | Manifest/generated sync, bundled/package reachability, in-flight state compatibility, stale compaction state |
|
Issue-lane (#4560 closure owner) independent exact-head corroboration — 2026-08-20T10:45Z UTC Read-only verification at exact head Status correction for the record: this PR is I re-derived the highest-severity findings from the head blobs rather than accepting the review prose, and four P1s plus one P2 are confirmed real defects:
Findings 3, 6, 7, 8, 9, 10 are consistent with the code I read and I have no basis to dispute them; finding 3 (reduced cohort still unconditionally requiring Terminal chain from here: resolve the P1/P2 set → integrate onto current Still outstanding on the issue's own acceptance list and not waived: the comparative baseline-vs-candidate evidence across small / medium-multi-goal / high-risk fixtures under no / one / repeated forced compaction, including Ralplan review, Ultragoal implementation, parallel executor work, parallel boundary cohort review, and blocker-fix/re-review. I close #4560 on verified evidence that the merge commit is on — |
|
Fix-forward escalation from the #4560 closure lane — 2026-08-20T11:50Z UTC This PR has been static for ~6.5h at head Verified state (recomputed this run)
Therefore the only real blocker is the review fix-forwardNot conflicts, not the digest, not CI, not reviewer availability. It is the 9 P1 + 2 P2 findings. I independently re-derived five from the head blobs (detail: #4562 (comment)) and they are real:
Plus the issue's unmet acceptance obligation: comparative baseline-vs-candidate evidence across small / medium-multi-goal / high-risk fixtures under no / one / repeated forced compaction, spanning Ralplan review, Ultragoal implementation, parallel executor work, parallel boundary cohort review, and blocker-fix/re-review. Sequenced ask for the #4562 owner lane
One correction for the record: the "requested reviewer / 0 reviews / reviewer is the blocker" framing in older #4560 receipts is stale — a non-author review was submitted and it requested changes. My lane made no mutation to this branch or worktree, created no duplicate implementation, and removed its trial worktree. #4560 stays open and closes the moment the merge commit is on — |
ee0ccfb to
3e66a40
Compare
|
Fix-forward complete — all 9 P1 + 2 P2 addressed at new exact head @snowykr re-review requested. Rebased onto current Findings addressedP1-2 multi-goal collapse — P1-4 security classifier — added P1-3 reduced cohort unusable — two parts. Added P1-1 intent bypass — planner-only and revision-only runs now project P1-5 in-flight cohort state — the compaction matrix asserts the joined cohort P1-6 symlinked ancestors — added P1-7 hash/project divergence — P1-8 stale source snapshot — added P2-10 stale workflow owner — P2-11 env leak — P1-9 forced-compaction matrix — new Verification
Body gate fixThe previous head also failed Evidence remains deterministic proxies. No identical-output, zero-drift, or live-provider latency claim is made. QA/verification, Design direction credit to @HaD0Yun. — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 7 actionable issues, led by Auth paths can be misclassified as low risk and CLI lane-selection proof cannot validate. These findings require changes before approval.
Findings / Required Changes
- [P1] Auth paths can be misclassified as low risk.
Reference:packages/coding-agent/src/gjc-runtime/ultragoal-validation-policy.ts:57-71
Filename markers omit generic auth names, so files such as auth.ts outside the hard-coded prefixes can qualify for reduced review. Add a conservative auth/security marker or fail closed for security-related paths. - [P1] CLI lane-selection proof cannot validate.
Reference:packages/coding-agent/src/gjc-runtime/ultragoal-runtime.ts:5320-5325
lane-selectionemits non-emptyreasonsfromapplicability.selection, but validation rejects every non-empty reasons array. Align the proof schema and add a CLI-to-validator round-trip test. - [P1] CLI-generated lane-selection proof is rejected by its validator.
Reference:packages/coding-agent/src/gjc-runtime/ultragoal-runtime.ts:2538-2544
lane-selection serializes applicability.selection (for example riskClass and basisUnchanged entries) into validationLaneSelection.reasons, but validation rejects every non-empty reasons array for low-risk proofs. Align the proof schema and validator, then add an end-to-end CLI-output-to-validate test. - [P1] Generated native bindings are not classified high risk.
Reference:packages/coding-agent/src/gjc-runtime/ultragoal-validation-policy.ts:164-171
isComputerControlSurfaceChangePathexcludes thegenerated-bindingcategory, and nopackages/nativeshigh-risk prefix exists; a generated native binding change can therefore receive the low-risk reduced cohort despite the documented native/platform safeguard. Treat generated bindings as high risk and add a regression test. - [P1] Lane-selection CLI proof is rejected.
Reference:packages/coding-agent/src/gjc-runtime/ultragoal-runtime.ts:2538-2551
quality-gate lane-selectionemits non-emptyreasonsviaapplicability.selection, but validation rejects every non-empty low-risk reasons array. Align the emitted proof and validator contract, then add an end-to-end CLI-to-validation test. - [P1] Repository witness misses content-only concurrent mutations.
Reference:packages/coding-agent/src/gjc-runtime/ultragoal-change-set.ts:359-382
repositoryStateWitness records only HEAD and porcelain status, so a tracked or untracked file can change contents while retaining the same status and evade captureDrifted; the raw diff/hash may then describe a source basis that never existed atomically. Include content digests in the before/after witness or retry/fail closed when any captured file changes. - [P1] Source hashes can omit untracked-file content.
Reference:packages/coding-agent/src/gjc-runtime/ultragoal-change-set.ts:54-78
computeUltragoalReviewSourceHash accepts a trusted change set with added paths and no untrackedContentHash, allowing an incomplete source basis to authenticate evidence reuse. Require verified untracked-content coverage whenever untracked files may be present, otherwise return undefined.
CI / Verification
- Reviewed the exact remote head:
3e66a40dd0009c9d9d061928abdd0d9311ace1f5. - CI summary: 28 passing, 2 failing, 10 pending/cancelled/skipped.
- Failing checks:
PR contract bootstrap,Validate exact-head PR contract. - Non-successful checks without pass evidence:
Telegram daemon generation guard,Affected path validation / plan,Affected path validation / darwin-arm64 tab-worker smoke,gjc-state-gates / ${{ matrix.group }},Affected path validation / ${{ matrix.key }},Affected path validation / native-build,Windows Telegram daemon safety,Windows dev:doctor + session-path regression. - Passing evidence reviewed:
Virtual integration validation,Affected path validation,Affected path validation / evidence producer,Affected path validation / ts-build:ts:Y29kaW5nLWFnZW50:cGFja2FnZXMvY29kaW5nLWFnZW50,Affected path validation / test:packages/coding-agent/test/session-manager-resident-cache.test.ts,Affected path validation / test:packages/coding-agent/test/notifications-live-stream.test.ts,Affected path validation / test:packages/coding-agent/test/ralplan-decision-artifacts.test.ts,Affected path validation / test:packages/coding-agent/test/gjc-runtime/workflow-manifest-phase-sets.test.ts. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | The runtime preserves high-risk lane selection, but the public lane-selection proof API is incompatible with its validator and cannot reliably cross the boundary. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | Correctness is mostly fail-closed, but concurrent content races can produce mixed source snapshots and the advertised lane-selection CLI proof is internally incompatible with validation. |
| A3. Security / Privacy / Trust | CHANGES_REQUESTED | A1-A5 security/privacy risk is established in validation risk classification and source-basis trust; structured recovery paths were reviewed for filesystem confinement and digest verification. |
| A4. Verification / Tests / CI | CHANGES_REQUESTED | Tests and CI establish broad passing validation, but an observable low-risk lane-selection proof regression prevents the new reduced-validation CLI path from functioning. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | Integration and documentation contracts are mostly aligned, but generated native bindings can bypass the documented high-risk validation policy; Windows/native behavior remains unverified. |
Limitations
- PR contract bootstrap and exact-head contract validation failed, so contract-compliance claims beyond the inspected implementation are not established.
- CI summary reports failures for PR contract bootstrap and exact-head PR contract validation, but provides no diagnostics, so their specific causes cannot be determined from brokered facts.
- Skipped Windows and native-build CI jobs prevent establishing platform behavior on those targets; successful affected-path checks do not replace those executions.
…validation (#4560) Long ralplan -> ultragoal runs compacted mid-flight with only a thin state projection (goal objective/status, phase, todos) plus a generic continuation prompt, losing the precise accepted scope/progress/evidence contract and inviting drift, duplicate review generations, and zero-progress continuation loops. Boundary validation also applied the full review cohort unconditionally, inflating token cost and failure surface on low-risk single-goal work. Add a structured workflow recovery projection derived from canonical durable state (Ralplan final/index.jsonl and Ultragoal goals.json/ledger.jsonl): bounded objective, accepted scope, non-goals, acceptance criteria, unresolved decisions, current goal, measurable progress, exact next action class, and digest. Compaction renders it into the state-aware context and the post-compaction auto-continue for active recognized workflows consumes it with scope-reload, user-intent-supremacy, no-silent-scope-expansion, unchanged-basis rerun-avoidance, and bounded zero-progress escalation rules. Malformed, stale, or tampered state degrades safely to the previous behavior, and paused/terminal/unknown workflows stay inert. Add a deterministic Ultragoal validation-applicability policy computed from durable facts (trusted change set, plan shape, open review blockers, joined-cohort source hash). Low-risk single-goal boundaries may omit redundant cleaner/architect ceremony and the duplicate terminal-critic read only through a validationLaneSelection proof that must exactly mirror the runtime-computed selection; QA, sourceHash binding, receipts, provenance, and join-before-repair stay mandatory. High-risk, multi-goal, computer/shared-registry, migration, and public-contract paths keep the full heavyweight cohort, and an unchanged immutable source/evidence basis permits reusing verified boundary evidence instead of forcing a full rerun. Co-authored-by: DoYunHa <102889891+HaD0Yun@users.noreply.github.com> Lore-id: 4a5f19c2 Constraint: preserve executor and cleaner||architect||qa cohort parallelism (#3473/#3474) Constraint: runtime-authoritative lane selection, never free-form model self-exemption Constraint: compaction reloads the durable contract; scope expansion must be classified, never silently accepted Rejected: model-declared lane skipping | prose cannot be validated fail-closed Rejected: second durable workflow-state store | projection is read-only over existing canonical state Confidence: high Scope-risk: moderate Reversibility: trivial Tested: 19 focused tests across projection, policy, gate selection, and compaction continuation Tested: regression suites for compaction, ultragoal runtime, review, critic gate, batch release (327 pass) Not-tested: live multi-run dogfood latency deltas (documented as follow-up evidence)
… recency (#4560) Adversarial exact-head self-review findings: priorVerifiedGeneration was declared on the applicability input but never consumed (dead contract surface), and the ralplan recovery run selection used a lexicographic name sort, which is arbitrary for non-monotonic run ids — recovery must project the run the session actually touched last, so select by newest run-directory mtime. Co-authored-by: DoYunHa <102889891+HaD0Yun@users.noreply.github.com> Lore-id: 77c3e8b1 Constraint: no dead contract surface on the validation policy input Confidence: high Scope-risk: narrow Reversibility: trivial Tested: focused recovery/policy/continuation + state-aware compaction suites (37 pass); coding-agent typecheck + biome clean
The reconstructed workflow refactor still trusted unconstrained plan paths, counted snapshot reads as separate compactions, and allowed self-authored cohort hashes to satisfy unchanged-basis critic reuse. It also left material intent reconciliation until after expensive Ralplan consensus. Confine and digest-check Ralplan artifacts, recover active consensus actions from durable state, bind cohort hashes to the captured source basis, classify workflow enforcement and credential surfaces as high risk, add a pre-consensus intent stage, and cover forced-compaction and blocker re-review paths. Co-authored-by: DoYunHa <102889891+HaD0Yun@users.noreply.github.com> Lore-id: 9e4b6c2a Constraint: QA, sourceHash, receipts, provenance, join-before-repair, recursion caps, and critic ceilings remain mandatory Constraint: workflow enforcement changes classify themselves as high risk Rejected: self-declared unchanged source hashes | cannot prove evidence freshness Rejected: post-consensus-only intent interview | spends review before resolving material scope Confidence: high Scope-risk: wide Reversibility: migration-needed Tested: 366 workflow tests plus 180 compaction tests; coding-agent typecheck; manifest/default/rebrand gates Not-tested: live multi-model latency distribution
Generation-two review found that the authoritative cohort digest was required but unavailable through a supported command, making honest quality-gate construction impossible. Recovery also still accepted Ralplan rows that omitted their recorded digest. Expose a read-only quality-gate source-hash command, require digest-bearing confined Ralplan rows, reject tampered mode/index state, hash untracked symlinks by link identity, preserve the legacy planner-to-architect edge, and make planning-stuck recovery terminal. Co-authored-by: DoYunHa <102889891+HaD0Yun@users.noreply.github.com> Lore-id: 3d8f7a11 Constraint: the leader must obtain sourceHash from the runtime on the frozen snapshot Constraint: current Ralplan writer receipts require matching SHA-256 provenance Rejected: diagnostic-only hash discovery | forces an edit/retry loop at the terminal gate Confidence: high Scope-risk: moderate Reversibility: trivial Tested: 147 focused workflow tests; coding-agent typecheck; live source-hash CLI invocation
Affected-task CI exports repository-wide CI_DEV_CHANGED_PATHS into every test process. Standalone temporary git repositories inherited those unrelated unknown-status paths, so the authoritative source-hash command correctly failed closed even though their own change sets were complete. Bind CI changed-path evidence only when the inspected repository is inside GITHUB_WORKSPACE, preserve known Git status over duplicate CI pathname-only rows, and cover both independent-repository isolation and canonical-workspace fail-closed participation. Co-authored-by: DoYunHa <102889891+HaD0Yun@users.noreply.github.com> Lore-id: 61af32d9 Constraint: unknown-status paths remain fail-closed in the authoritative workspace Rejected: deleting CI_DEV_CHANGED_PATHS only in the test | leaves production nested-repository hashing environment-dependent Confidence: high Scope-risk: narrow Reversibility: trivial Tested: validation-lanes under representative CI_DEV_CHANGED_PATHS/GITHUB_WORKSPACE; change-set tests; ultragoal runtime/review; coding-agent typecheck
The production workspace binding correctly ignored outer CI_DEV_CHANGED_PATHS for independent repositories, but several Ultragoal suites intentionally use that variable as synthetic change-set input for temporary repositories. GitHub's outer GITHUB_WORKSPACE made those fixtures indistinguishable from accidental environment leakage and caused the authoritative runtime shard to fail. Clear and restore GITHUB_WORKSPACE only around the suites that explicitly simulate CI_DEV_CHANGED_PATHS fallback, leaving production unknown-status and canonical-workspace behavior unchanged. Co-authored-by: DoYunHa <102889891+HaD0Yun@users.noreply.github.com> Lore-id: 7b149ac0 Constraint: test isolation must not alter production fail-closed hashing Rejected: weakening workspace equality in production | reintroduces the original nested-repository leak Confidence: high Scope-risk: narrow Reversibility: trivial Tested: ultragoal-runtime under representative GitHub CI env 181 pass; related synthetic CI suites 35 pass; scope suites 20 pass; coding-agent typecheck
…mmar (#4560) Dev's #4699 follow-ups moved every ralplan write-path command to the --worktree-root form; the intent-stage line added by this branch still used the old positional form and tripped the bundled-skill write-path contract test after the transplant onto current dev. Lore-id: 4560-intent-write-grammar Constraint: must match dev's ralplan write-path command grammar exactly Tested: default-gjc-definitions 33/33; ralplan suites 142/142 Confidence: high Scope-risk: narrow Reversibility: trivial
A durable run directory symlink could redirect recovery reads outside the session plan root even when the recorded artifact path was relative.\n\nReject symlinked run directories before realpath resolution and cover the fail-closed recovery path with a regression test.\n\nLore-id: 4562-symlink-recovery\nConstraint: recovery must never trust a symlinked durable run directory\nTested: bun test packages/coding-agent/test/workflow-recovery-projection.test.ts\nConfidence: high\nScope-risk: narrow\nReversibility: trivial\nDirective: preserve fail-closed recovery on malformed or redirected durable state,cwd:/home/bellman/Workspace/gajae-code-pr-4562-current-exact-owner,timeout:120}
…#4560) Snowykr's exact-head review found the fail-closed claims were not actually fail-closed: multi-goal risk was derived from remaining goals, so it evaporated at the final aggregate boundary; the security classifier was a hand-maintained prefix list that missed real credential surfaces; and the advertised architect omission was unreachable because architect evidence stayed unconditionally required with no supported way to obtain the selection proof. Recovery had matching gaps: a planner-only run resumed into Architect/Critic consensus against an unreconciled draft, only the leaf run directory was checked for symlinks, and the artifact was read for projection then reopened for hashing. Derive multiGoal from the durable aggregate shape; classify credential surfaces by filename marker plus explicit paths; expose the computed proof through `quality-gate lane-selection` and make architect evidence conditional on lane applicability; resume planner/revision at intent reconciliation; verify every path component with no-follow semantics and hash the projected bytes; re-check a repository state witness so a raced capture fails closed. Lore-id: 4560f1x2 Constraint: reduced lanes must never remove QA, sourceHash, receipts, or join Constraint: any unprovable condition must grade high-risk Rejected: extend the prefix list only | new auth files would silently regrade low Rejected: error when architect evidence accompanies an omitted lane | extra proof is harmless Confidence: high Scope-risk: wide Reversibility: safe Tested: final goal of a two-goal run, three named auth surfaces, planner-only recovery, symlinked ancestor, forced-compaction matrix across small/multi-goal/high-risk under 0/1/3 compactions Not-tested: multi-run live model behavior; evidence is deterministic proxies only Co-authored-by: HaD0Yun <HaD0Yun@users.noreply.github.com>
Risk-proportional validation must authenticate the complete source boundary and keep its lane-selection proof contract identical from CLI emission through validation.\n\nContent witnesses now bind every tracked and non-ignored untracked path, while generic auth/security and native surfaces remain on the full-risk lane. Issue: #4560\nPR: #4562\nTested: focused risk, witness, CLI round-trip, and compaction suites\nTested: bun run check\nNot-tested: tmux integration requires a verifiable local tmux server\nConfidence: high\nScope-risk: wide\nReversibility: revert-commit
3e66a40 to
ee9928b
Compare
|
Fix-forward head pushed: ee9928b Exact source digest: sha256:aa1275e776b1192595c6dc45e02d9397882f64e824658eb9baa756b5bbac2fe9 Verification: focused workflow suite 374 pass; full gjc-runtime suite 1259 pass; native/platform suite 84 pass, 2 skipped; package check passes. Full repository check reaches the current-dev SDK canonicalization gate; isolated timed compaction tests pass 18/18. Unrelated packages/tui QA artifacts were quarantined outside the worktree and excluded from this push. Fresh exact-head review requested from snowykr. |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 7 actionable issues, led by Canonicalize source-hash path ordering and Failing computer red-team CI gate. These findings require changes before approval.
Findings / Required Changes
- [P1] Canonicalize source-hash path ordering.
Reference:packages/coding-agent/src/gjc-runtime/ultragoal-change-set.ts:54-78
computeUltragoalReviewSourceHash hashes paths in input order, so equivalent change sets assembled with different ordering produce different sourceHash values and invalidate otherwise reusable cohort evidence. Sort canonical path/status rows before hashing and add an order-independence test. - [P1] Failing computer red-team CI gate.
Reference:packages/coding-agent/test/gjc-runtime/computer-red-team-fixtures.test.ts:238-256
CI reports this test failure; reproduce the failing assertion, fix the implementation or fixture, and rerun the affected validation before merge. - [P1] Filename-based auth detection misses embedded auth names.
Reference:packages/coding-agent/src/gjc-runtime/ultragoal-validation-policy.ts:89-103,138-151
Marker matching only checks listed substrings such as auth-storage and oauth; names like my-auth-helper.ts can classify low-risk despite being credential/security code. Add conservative auth/security filename matching and tests for embedded markers. - [P1] Normalize CI paths before merging with Git paths.
Reference:packages/coding-agent/src/gjc-runtime/ultragoal-change-set.ts:38-44,208-220
normalizeChangeSetPath strips only './', while CI metadata can retain backslash separators. On Windows, the same file can become a separate unknown row beside Git's slash-form row, forcing source-hash failure and changing gate behavior. Normalize separators at ingestion and merge by canonical paths. - [P1] Recovery artifact symlink race is not protected on Windows.
Reference:packages/coding-agent/src/gjc-runtime/workflow-recovery-projection.ts:139-142
O_NOFOLLOW is platform-dependent and may be unavailable on Windows, while the preceding path checks are raceable. Use a Windows-safe no-follow/open-and-verify strategy or explicitly fail closed on unsupported platforms, with Windows regression coverage. - [P1] Repository witness leaves a post-check window for concurrent mutation.
Reference:packages/coding-agent/src/gjc-runtime/ultragoal-change-set.ts:328-347
The final repositoryStateWitness is compared before returning, but the raw diffs and content hashes were read earlier and the returned change set is consumed afterward. A tracked file changed after the final witness can therefore pass the comparison while rawDiff/sourceHash still describe the prior bytes. Build the source basis from an immutable Git/filesystem snapshot, or perform an atomic/retried capture that verifies the witness immediately against the exact bytes used. - [P1] Tracked additions cannot obtain an authoritative source hash.
Reference:packages/coding-agent/src/gjc-runtime/ultragoal-change-set.ts:70-77
computeUltragoalReviewSourceHash requires untrackedContentHash for every added path, but computeCheckpointChangeSet hashes only paths returned by git ls-files --others; staged or committed additions are absent from that set. A normal change set containing a tracked added file therefore returns undefined and cannot use source-hash or low-risk validation. Hash all added-file content, not only untracked files, or distinguish tracked additions from untracked additions.
CI / Verification
- Reviewed the exact remote head:
ee9928bc27379213267f68d6049126fcba7ddc7a. - CI summary: 24 passing, 5 failing, 12 pending/cancelled/skipped.
- Failing checks:
Affected path validation,Affected path validation / evidence producer,Affected path validation / test:packages/coding-agent/test/gjc-runtime/computer-red-team-fixtures.test.ts. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | API and compatibility boundaries are mostly fail-closed, but non-canonical path ordering and separators can destabilize source identity and cross-platform validation behavior. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | Correctness and concurrency are not fully safe: tracked additions are rejected from authoritative hashing, and repository capture retains a TOCTOU mutation window. |
| A3. Security / Privacy / Trust | APPROVED | Security and privacy controls appear fail-closed for source integrity, authentication-sensitive paths, symlinks, and durable recovery data; no concrete exploitable issue was established. |
| A4. Verification / Tests / CI | CHANGES_REQUESTED | A4 conclusion: focused tests mostly pass, but the failed computer red-team validation prevents establishing a regression-free change. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | Integration and documentation behavior is covered, but Windows no-follow semantics and broad security-path classification remain established risks. |
Limitations
- CI summary reports computer-red-team and affected-path failures, but brokered evidence contains no job logs, so their causes cannot be diagnosed; Windows platform jobs were skipped.
2 participants
Summary
Closes #4560 with the complete workflow-efficiency contract reconstructed onto current
dev:intentstage resolves material objective/scope/non-goal/acceptance/verification questions before Architect/Critic consensus. Post-consensus intent verification is delta-only, and the legacyplanner → architectedge remains for in-flight pre-upgrade runs.planning_stuckrecovery. Ultragoal restores goal progress, blockers, joined cohort generation/hash, and exact next actions. Zero-progress escalation increments once per compaction attempt, not per snapshot read.gjc ultragoal quality-gate source-hash --jsonexposes the same authoritative integration-base/merge-base/path/diff/untracked-content digest the checkpoint validator enforces. Untracked symlinks are hashed by link identity without following external targets.Design direction credit: @HaD0Yun.
Safety invariants preserved
sourceHashbinding; model-authored equal strings cannot prove unchanged evidence.Verification
Frozen reconstruction:
sha256:21ce0fde67443802b30e2ab17c6e2daf4f589a6d39932b1c9b5a5c5a33f842d4Executed evidence:
artifacts/pr4562-final-tests.xml).CI_DEV_CHANGED_PATHS+GITHUB_WORKSPACE: 9 pass, 0 fail; outer-workspace paths are ignored for an independent nested Git root while canonical workspace unknown-status evidence remains fail-closed.bun --cwd=packages/coding-agent run check:types— passed.bun scripts/check-visible-definitions.ts— passed.bun scripts/rebrand-inventory.ts --strict— passed.bun test packages/coding-agent/test/default-gjc-definitions.test.ts— 29 pass, 0 fail.Known repository-wide external defect:
bun run check:tsreaches the pre-existingscripts/telegram-daemon-generation-guard.test.tsfixture lookup failure forpackages/coding-agent/test/notifications-topic-registry.test.ts; the standalone guard passes from repository root. This PR does not suppress or work around it.Comparative / forced-compaction evidence
Deterministic regression fixtures cover low-risk lane reduction versus unchanged full-cohort high-risk paths, forced compaction during Ralplan review, Ultragoal execution, parallel review/executor work, and blocker-fix re-review. They assert durable contract/action recovery and fail-closed source/risk selection; they do not claim identical model outputs or measured live-provider latency.
Risk classification
low-risk— ordinary fix/maintenance; the repository owner may use the explicitmerge-self-approvedsolo verdict (no independent human review; the verdict name itself records this) with a risk-record comment bound to the exact head.regression-risk— fix with material regression risk; requires one assigned independent domain reviewer whose authenticated exact-headAPPROVEDreview the gate verifies (extra:independent:<login>; the token alone never suffices).high-risk— large refactor, feature, or materially high-risk change (security/auth/install/remove/public API/destructive lifecycle/architecture); requires one assigned independent domain reviewer with an authenticated exact-headAPPROVEDreview (extra:independent:<login>).Review state
The PR branch is rebased onto current dev ba7096f and carries exact head ee9928b.
gajae.pr-review-verdict.v1 needs-human sha256:bb7d98a735f2011a0b89dc29e5e065d2a7d15a6367f4d013e0a70b5a1f8e89d8 reviewer:human reviewer-id:snowykr evidence:awaiting fresh independent snowykr review on exact head ee9928b